-
Notifications
You must be signed in to change notification settings - Fork 117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[JENKINS-32526] - Handle null @AncestorInPath when selecting Jobs/Dow… #79
Conversation
…nstream/Upstream Jobs and for autocompletion.
@reviewbybees |
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
@@ -577,7 +577,9 @@ public String resolve(String name) { | |||
public static final class DescriptorImpl extends BuildStepDescriptor<Builder> { | |||
|
|||
public FormValidation doCheckProjectName( | |||
@AncestorInPath Job<?,?> anc, @QueryParameter String value) { | |||
@AncestorInPath AbstractProject anc, @QueryParameter String value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need have it AbstractProject
?
Pipeline (aka. workflow)'s snippet generator requires Job
.
f28c554
Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests. |
…ass back to Job, added message about the context
@@ -188,6 +188,11 @@ public FormValidation doCheckUpstreamProjectName( | |||
if (containsVariable(upstreamProjectName)) { | |||
return FormValidation.ok(); | |||
} | |||
|
|||
if (project == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you are it, you should replace AbstractProject
with Job
to get Pipeline compatibility missed in #55. Ditto in other related validation methods. CC @apemberton: currently with Pipeline this throws a NullPointerException
; fixing the PR as written would make it unconditionally return OK; also fixing the type of the parameter would make it actually perform the desired validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JENKINS-33578 specifically.
@jglick Changed the variable type to |
if(project != null && project instanceof AbstractProject) { | ||
upstreamRoot = ((AbstractProject) project).getRootProject(); | ||
} else { | ||
upstreamRoot = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you want to set it to project
. (getRootProject
defaults to returning this
.)
BTW if you want to test Pipeline compatibility of completions, |
@@ -1,3 +1,4 @@ | |||
CopyArtifact.AncestorIsNull=Ancestor/Context Unknown: the project specified cannot be validated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is helpful for users.
How about a text like "You look configuring non-buildable job (may be template) ..."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it is not exactly “non-buildable”. But certainly the term “ancestor” will be meaningless to a user (this is an API term). I think it suffices to say that the configuration form is being displayed without enough context to determine which job will host the build step.
… to find upstream project.
You'd better add a comment when you add a new commit as github doesn't send a notification for commits. |
@@ -196,7 +196,7 @@ public FormValidation doCheckUpstreamProjectName( | |||
} | |||
|
|||
AbstractProject<?,?> upstreamProject = jenkins.getItem( | |||
upstreamProjectName, project.getRootProject(), AbstractProject.class | |||
upstreamProjectName, project, AbstractProject.class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might misunderstand the comment by jglick (https://github.com/jenkinsci/copyartifact-plugin/pull/79/files/65b30d6da1983e11be0f93d9defb664796fca99d#r56505288)
That meant you don't need to use null
for project
even when project
doesn't have getRootProject
.
getRootProject
should be still required if it's available.
Though I don't know an actual case resulting project.getRootProject() != project
in this context, it's better to implement in the same way as DownstreamBuildSelector
works.
…ip validation when null.
@ikedam You're right I misunderstood jglick's point. I reimplemented the use of |
Thanks for the update. |
…nstream/Upstream Jobs and for autocompletion.
JENKINS-32526